Skip to content

NO-JIRA | fix: refactor inspector to use pipelines#175

Open
AvielSegev wants to merge 1 commit intokubev2v:mainfrom
AvielSegev:adopt-pipelines-seperate-inspector-and-inspection
Open

NO-JIRA | fix: refactor inspector to use pipelines#175
AvielSegev wants to merge 1 commit intokubev2v:mainfrom
AvielSegev:adopt-pipelines-seperate-inspector-and-inspection

Conversation

@AvielSegev
Copy link
Collaborator

Signed-off-by: Aviel Segev asegev@redhat.com

@AvielSegev AvielSegev requested a review from a team as a code owner March 18, 2026 11:08
@AvielSegev AvielSegev requested review from amalimov and ronenav March 18, 2026 11:08
@AvielSegev AvielSegev force-pushed the adopt-pipelines-seperate-inspector-and-inspection branch 16 times, most recently from e5bbb23 to 7ca731b Compare March 18, 2026 15:29
@AvielSegev AvielSegev force-pushed the adopt-pipelines-seperate-inspector-and-inspection branch 4 times, most recently from 3dbfb44 to 348a8bc Compare March 19, 2026 14:05
InspectionWorkBuilder func(id string) []models.WorkUnit[models.InspectionStatus, models.InspectionResult]
)

type InspectionService struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't want to export this service, you should do it the other way around.
inspectionService + methods public.
Currently, we're exporting the service without any public methods.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop all the mutex from this service. If the InspectorService is the only caller, it should be responsible for protecting it.
You can have problems with redundant locking or even worst having some locking order problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop all the mutex from this service. If the InspectorService is the only caller, it should be responsible for protecting it.
You can have problems with redundant locking or even worst having some locking order problems.

done:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't want to export this service, you should do it the other way around.
inspectionService + methods public.
Currently, we're exporting the service without any public methods.

Can you explain why ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. What is your intention?
You exported the type InspectionService but type itself has no public methods.
The type has public methods, but the type itself is not public at the package level.

@AvielSegev AvielSegev force-pushed the adopt-pipelines-seperate-inspector-and-inspection branch from 348a8bc to 44cf33c Compare March 19, 2026 14:39
models.InspectionStatus{State: s}); err != nil {
return fmt.Errorf("updating vm %s in store: %w", vmID, err)
for _, p := range pipelines {
for p.IsRunning() {
Copy link
Collaborator

@tupyy tupyy Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong.
I don't understand what the intent of this method is, but if this is the watch method we talked about is completely wrong.

  • you don't see new pipelines.
  • you wait for pipeline 1 to complete, wait 2 secnds than pass to pipeleine 2 and next..
  • select is wrong. This is a non-blocking check on ctx.Done(). If the context isn't canceled at the exact instant the select runs, it falls through to default and sleeps for 2 seconds. During that sleep, context cancellation is completely ignored.
select {
  case <- ctx.Done():
     return
  case <- time.After(2 * time.Second):
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you
I've changed the method and fix the problems here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is my version:

func (i *InspectorService) run(ctx context.Context) {
	defer func() {
        ticker.Stop()

		i.inspectionSvc.stop()
		_ = i.closeVsphereClient(ctx)
		
        if cancel {
			i.setState(models.InspectorStateCanceled)
        } else {
		    i.setState(models.InspectorStateCompleted)
        }
        close(i.stop)
	}()
	i.setState(models.InspectorStateRunning)
	ticker := time.NewTicker(5 * time.Second)
    cancel := false

	for {
		select {
		case <-i.stop:
            cancel = true
			return
		case <-ticker.C:
			if !i.inspectionSvc.isBusy() {
				return
			}
		}
	}
}

run stops everything: inspection services and sets the status to canceled or completed.
Please protect setState and getState with their own mutex, separate from the one protecting Start, Add, and Stop (see collector), to avoid deadlocks. (Stop acquire the lock, setState tries to acquire lock while Stop holding the lock -> deadlock)
Also, inspectionService should protect its resources (map of pipeline). isBusy and Add should be protected inside inspectionSrv.


c.setState(models.InspectorStateCompleted)
zap.S().Info("inspector finished work")
func (i *InspectorService) WithInspectionService(svc *InspectionService) *InspectorService {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if inspectionSrv is owned by InspectorService it should be created inside, not here. What if the InspectorSrv is created without inspectionSrv?
I think now I understand why InspectionSrv is exported and the methods are private.

Copy link
Collaborator Author

@AvielSegev AvielSegev Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more for testing purposes:
Similar to what we are doing with the buildFn:


	if i.inspectionSvc == nil {
		i.inspectionSvc = NewInspectionService()
	}

	if i.buildFn == nil {
		i.buildFn = i.buildInspectorWorkUnits
	}

@AvielSegev AvielSegev force-pushed the adopt-pipelines-seperate-inspector-and-inspection branch 26 times, most recently from 5fdbf23 to 0141fa2 Compare March 19, 2026 20:17
Signed-off-by: Aviel Segev <asegev@redhat.com>
@AvielSegev AvielSegev force-pushed the adopt-pipelines-seperate-inspector-and-inspection branch from 0141fa2 to 6d14952 Compare March 19, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants